refactor!: Governance collateral/fee is actually a commitment #6182
Conversation
| @@ -30,10 +30,10 @@ extern RecursiveMutex cs_main; | |||
| static constexpr double GOVERNANCE_FILTER_FP_RATE = 0.001; | |||
|
|
|||
|
|
|||
| static constexpr CAmount GOVERNANCE_PROPOSAL_FEE_TX = (1 * COIN); | |||
| static constexpr CAmount GOVERNANCE_COMMITMENT_AMOUNT = (1 * COIN); | |||
| static constexpr int64_t GOVERNANCE_COMMITMENT_CONFIRMATIONS = 6; | |||
There was a problem hiding this comment.
@UdjinM6, can you explain the rationale/purpose for the 1 and 6 block confirmations here? Does it make more sense now to require InstantSend and/or ChainLocks for proposal validity instead of block confirmations (in a separate issue/PR)?
There was a problem hiding this comment.
We had no DML (InstantX wasn't as reliable as deterministic InstantSend) and no ChainLocks (reorgs up to 6 blocks could happen easily) back then. We could indeed consider proposals that have their commitments IS-/ChainLocked safe to accept without waiting for 6 confs now. I'd probably still require at least 1 conf to keep the logic simple because handling "orphan" proposals can be complicated (error/bug-prone) imo.
UdjinM6
force-pushed
the
refactor_gov_collateral_fee
branch
2 times, most recently
from
55c1e6a
to
b310736
Compare
|
Since this has come to a code level change, I'd vote for allowing collateral to be used (tap into the coinjoin collateral verification function) while also keeping backwards compatibility of burning coins. I'd also vote for using the industry standard terminology of "burning" rather than "commitment" so that it's clear what's going on, but at least it's not overloading terms, so that's great. |
There was a problem hiding this comment.
breaking in multiple interfaces
obj.pushKV("collateralHash", collateralHash.ToString());
obj.pushKV("commitmentHash", m_commitment_hash.ToString());
{"fee-txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "txid of the corresponding proposal fee transaction"},
{"commitment-hash", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "txid of the corresponding proposal commitment transaction"},
bObj.pushKV("CollateralHash", govObj.GetCollateralHash().ToString());
bObj.pushKV("CommitmentHash", govObj.GetCommitmentHash().ToString());
obj.pushKV("proposalfee", ValueFromAmount(GOVERNANCE_PROPOSAL_FEE_TX));
obj.pushKV("commitmentamount", ValueFromAmount(GOVERNANCE_COMMITMENT_AMOUNT));
a few things so far:
- I think the value here is probably low, sure it's not really a "collateral," and probably "commitment" is better term, but I don't really think it's high value to fix.
- we would for sure need to wait for a major version for this, but when possible, we try to deprecate something in a version, which is very easy to "fix" by adding a launch option and then actually remove later. I don't see an easy way to keep these changes as backwards compatible as done here, and the number of services / tools this would probably break, makes these breaking changes probably not worth it to me.
|
This pull request has conflicts, please rebase. |
UdjinM6
force-pushed
the
refactor_gov_collateral_fee
branch
from
b310736
to
6287c14
Compare
|
This pull request has conflicts, please rebase. |
Issue being fixed or feature implemented
dashpay/docs-core#108
What was done?
"collateral"/"fee" -> "commitment"
How Has This Been Tested?
Run tests
Breaking Changes
Various governance RPCs are affected:
Checklist: